Skip to content

Conversation

@staskus
Copy link
Contributor

@staskus staskus commented Aug 25, 2025

Description

WOOMOB-1131

Infrastructure for remotely fetching required data from API, pagination, and in-memory cache. Implement a remote-first approach with the 25 most recent POS orders, automatic pagination when scrolling to the bottom, and in-memory caching for immediate display.

For consistency, I implemented a similar approach that we use with the items. This PR contains a lot of boilerplate, but I thought it's easier to do it in one go.

  • Service + Strategy + Controller + OrdersRemote combination. The strategy will be expanded with a search later.
  • Controller implements simple in-memory caching, refresh, and pagination
  • Created PointOfSaleOrdersModel since this view state is completely separate from the aggregate model (and wider POS)
  • Separate POS-only entities: POSOrder, POSOrderItem, POSOrderRefund
  • Dummy AI-generated UI for List and Details (it's out of scope for this PR, only for easier testing)

Steps to reproduce

  1. Open POS -> Menu -> Orders
  2. Confirm the loading state appears when orders are loading
  3. Confirm correct POS orders are loaded, compare with wp-admin and Woo app orders
  4. Confirm pull-to-refresh fetches new data
  5. Confirm scrolling down triggers pagination
  6. Close the orders view and reopen
  7. Confirm cached data is immediately shown

UI is out of scope, generated to facilitate testing

Testing information

Tested on iPad Air M2 18.5 simulator

Screenshots

Order.List.and.Details.mov

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@staskus staskus added this to the 23.2 milestone Aug 25, 2025
@staskus staskus added feature: POS type: task An internally driven task. labels Aug 25, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented Aug 25, 2025

1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 25, 2025

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Numberpr16036-7ff28cc
Version23.1
Bundle IDcom.automattic.alpha.woocommerce
Commit7ff28cc
Installation URL3i4b2h25qitio
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@iamgabrielma
Copy link
Contributor

👋 I'll review this one later today, for the time being seems the test target won't compile:

Value of type 'any POSOrdersRemoteProtocol' has no member 'loadPOSOrders'

Periphery seems to complain as well but there is no output in buildkite, perhaps because of the build failure hitting before the scan. If needed objects can be marked with // periphery:ignore temporarily to pass CI.

@iamgabrielma iamgabrielma self-assigned this Aug 26, 2025
@staskus
Copy link
Contributor Author

staskus commented Aug 26, 2025

@iamgabrielma yes, sorry! I need to get used to periphery. In these internal project PRs, I sometimes implement something that is supposed to be used in another PR but is not yet used now. I'll fix those warnings

@staskus staskus marked this pull request as draft August 26, 2025 06:28
@staskus staskus marked this pull request as ready for review August 26, 2025 08:11
Copy link
Contributor

@iamgabrielma iamgabrielma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well and LGTM! Just left a couple of non-blocking comments.

✅ Confirm the loading state appears when orders are loading
✅ Confirm correct POS orders are loaded, compare with wp-admin and Woo app orders
✅ Confirm pull-to-refresh fetches new data
✅ Confirm scrolling down triggers pagination
✅ Close the orders view and reopen
✅ Confirm cached data is immediately shown

import struct NetworkingCore.Order
import protocol NetworkingCore.POSOrdersRemoteProtocol

public final class PointOfSaleOrderService: PointOfSaleOrderServiceProtocol {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought, the naming of PointOfSaleOrderService is pretty similar to the existing POSOrderService and protocols which may lead to confusion. I have no specific suggestion at the moment, but I think we should start to differentiate them more, perhaps renaming to PointOfSaleOrdersService for now would suffice 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. We could call it OrderList{Controller/Service/etc..} so it would be as clear as possible...

return .init(items: posOrders,
hasMorePages: pagedOrders.hasMorePages,
totalItems: pagedOrders.totalItems)
} catch AFError.explicitlyCancelled {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL that we will separate these two, what's the use case for the request being explicitly cancelled? We may cancel it for performance/cache reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was more related to the search #15580, where inputting more letters would cancel the previous request. However, we don't want to show any error, so we explicitly handle and treat the request as successful.

func loadNextOrders() async
}

@Observable final class PointOfSaleOrdersController: PointOfSaleOrdersControllerProtocol {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: PointOfSaleOrderController vs PointOfSaleOrdersController are pretty close

default:
return "Unknown Order"
}
@Environment(PointOfSaleOrdersModel.self) private var ordersModel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the usage of this model by injecting directly into the environment, rather than passing it through the aggregate model. Would you say it breaks the existing design or we should be fine to adopt it? Mainly asking because I tried the same approach for POS settings, but ultimately went through the aggregate model since I needed related services that were already injected.

PD: It could be just because UI is temporary, sorry if that's the case :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I injected it separately because it didn't have any connection to the existing aggregate model. It looked like a completely different feature that depends on different kinds of information. Settings could be a different case since it makes sense that it shares dependencies with the main part of the POS.

private let purchasableItemsSearchController: PointOfSaleSearchingItemsControllerProtocol
private let couponsController: PointOfSaleCouponsControllerProtocol
private let couponsSearchController: PointOfSaleSearchingItemsControllerProtocol
private let ordersController: PointOfSaleOrdersControllerProtocol
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here with ordersController vs orderController, maybe makes sense to rename the original one in the sense of order creation, fulfilment, factory, ... and this one as order list? Naming is indeed a difficult problem 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points, I'll rename!

@staskus staskus merged commit 9d7f634 into trunk Aug 27, 2025
14 checks passed
@staskus staskus deleted the woomob-1131-woo-poshistorical-orders-orders-fetching branch August 27, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: POS type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants